-
Notifications
You must be signed in to change notification settings - Fork 10
Improve test coverage for Python support #213
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Improve test coverage for Python support #213
Conversation
|
@phlexbot format |
|
Automatic cmake-format fixes pushed (commit e60d811). |
|
@phlexbot format |
|
No automatic cmake-format fixes were necessary. |
|
Automatic clang-format fixes pushed (commit 2aa7957). |
|
@phlexbot python-fix |
|
Automatic Python linting fixes pushed (commit 092dec4). |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (73.68%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. @@ Coverage Diff @@
## main #213 +/- ##
==========================================
+ Coverage 75.69% 80.12% +4.42%
==========================================
Files 125 125
Lines 2946 3074 +128
Branches 519 546 +27
==========================================
+ Hits 2230 2463 +233
+ Misses 497 383 -114
- Partials 219 228 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
38fc738 to
47661fa
Compare
|
@phlexbot format |
|
No automatic clang-format fixes were necessary. |
|
Automatic cmake-format fixes pushed (commit bb954c8). |
|
@phlexbot python-fix |
|
Automatic Python linting fixes pushed (commit cfdb09a). |
c9ccf06 to
1159796
Compare
|
@phlexbot format |
|
Automatic clang-format fixes pushed (commit b145a22). |
|
@phlexbot format |
|
Automatic cmake-format fixes pushed (commit cc928e7). |
|
No automatic clang-format fixes were necessary. |
36bd016 to
e1d2b41
Compare
|
@phlexbot clang-fix |
|
Automatic clang-format fixes pushed (commit 81bff12). |
|
Review the full CodeQL report for details. |
1 similar comment
|
Review the full CodeQL report for details. |
|
@phlexbot clang-fix |
|
Automatic clang-format fixes pushed (commit 281a74f). |
|
Review the full CodeQL report for details. |
1 similar comment
|
Review the full CodeQL report for details. |
|
@phlexbot clang-fix |
|
No automatic clang-format fixes were necessary. |
|
Review the full CodeQL report for details. |
wlav
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've given all the relevant feedback I can at this point. :)
|
@phlexbot clang-fix |
|
Automatic clang-format fixes pushed (commit 72cb86c). |
|
Review the full CodeQL report for details. |
2 similar comments
|
Review the full CodeQL report for details. |
|
Review the full CodeQL report for details. |
Goal
The primary goal of this PR is to significantly improve the test coverage for the Phlex Python plugin and resolve several discovered issues related to memory management, type conversion, and core graph connectivity safety. It also modernizes the build system configuration and coverage tooling.
Key Changes and Rationale
1. Introduction of
phlex.VariantHelperVariantinplugins/python/python/phlex/__init__.py.__annotations__might not match the positional labels defined in the Phlex configuration.Variantclass allows developers to wrap a callable and associate it with custom annotations and a specific name. This enables the same Python function to be registered multiple times with different types without modifying the original function.phlex_callablefor performance while still using the provided annotations for type mapping.2. Robustness Improvements in
modulewrap.cppPy_DECREFcalls in the error paths ofparse_args,md_transform, andmd_observe. This prevents Python object leaks when registration fails.PySequence_Fast. This allows Phlex to accept any Python sequence (lists, tuples, etc.) for input/output labels.double64]]instead offloat64]].ndarray | list).list[int],list[float], andlist[double].3. Core Connectivity & Safety
phlex/core/edge_maker.cppandphlex/core/edge_maker.hpp.std::runtime_errorwith a descriptive message identifying the offending nodes/ports.4. Build System & Tooling Modernization
DART_TESTING_TIMEOUTandCTEST_TEST_TIMEOUTto prevent long stalls in CI environments.scripts/coverage.shand its documentation to support both Clang (LLVM source-based) and GCC (gcov) presets, enabling both high-fidelity local reports and CI-compatible XML outputs.plugins/python/README.md(architecture guide) and.github/copilot-instructions.md(project context for AI assistants).New Tests
This PR adds a comprehensive suite of tests in
test/python/to ensure long-term stability:unit_test_variant.py: Dedicated unit tests for theVariantclass logic.test_types.py&test_coverage.py: Exercise various scalar, vector, and list-based type conversions to ensure data integrity.test_callbacks.py: Targets edge cases inmodulewrap.cpp, such as 3-argument callbacks and Python exception propagation.pybadbool.jsonnet,pybadint.jsonnet,pymismatch.jsonnet, etc.) that verify Phlex correctly catches and reports type and signature mismatches.test/python/CMakeLists.txtto supportpytest-cov, enabling integrated Python coverage reporting.